Tune Newton MJWarp locomotion environments - #6861
Conversation
Greptile SummaryThis PR retunes Newton MJWarp contact and solver settings for velocity locomotion, adjusts AnymalD initialization, and adds a maximum body-speed termination.
Confidence Score: 4/5The PR should not merge until the nonzero Newton rough-terrain contact margin is restored or replaced with robot-specific margins that preserve stable triangle-mesh contact. The shared configuration now applies a zero collision margin to Newton rough-terrain robots despite the prior documented requirement for nonzero margins, and its newly explicit contact capacity becomes stale when supported configuration paths change the environment count. Files Needing Attention: source/isaaclab_tasks/isaaclab_tasks/core/velocity/velocity_env_cfg.py and source/isaaclab_tasks/isaaclab_tasks/core/velocity/config/anymal_d/rough_env_cfg.py Important Files Changed
Reviews (1): Last reviewed commit: "fix locomotion" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # terrain — without it, non-AnymalD robots fail to learn stable contact | ||
| # on triangle-mesh terrain. See isaaclab_newton 0.5.22 changelog. | ||
| default_shape_cfg=NewtonShapeCfg(margin=0.01), | ||
| default_shape_cfg=NewtonShapeCfg(margin=0.0, ke=160000.0, kd=1100.0), |
There was a problem hiding this comment.
When inherited rough-terrain tasks run with the newton_mjwarp preset, this shared zero margin removes the nonzero margin required for stable triangle-mesh contact, causing affected robots to lose contact stability and fail to learn rough-terrain locomotion. It also combines with the removed AnymalD override to discard that robot's separately tuned margin.
Knowledge Base Used: isaaclab_tasks: Task Registration and Organization
| self.sim.render_interval = self.decimation | ||
| self.sim.physics_material = self.scene.terrain.physics_material | ||
| newton = self.sim.physics.newton_mjwarp | ||
| newton.collision_cfg.rigid_contact_max = newton.solver_cfg.nconmax * self.scene.num_envs |
There was a problem hiding this comment.
rigid_contact_max is calculated from the construction-time environment count, but play mode and CLI/Hydra overrides change scene.num_envs afterward without recalculating it. This leaves play mode with a substantially oversized contact allocation and larger runs with a contact budget that no longer matches the requested number of environments.
Knowledge Base Used: isaaclab_tasks: Task Registration and Organization
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
The PR adds a public body-speed termination and retunes the shared Newton/MJWarp rough-locomotion configuration. Before merge, the shared zero-margin tuning should be scoped or justified against the documented dependency of other robots, contact-buffer sizing must account for the final environment count, public API documentation needs parameter units, and both touched packages need changelog fragments.
- Design and architecture: The termination is appropriately placed with the existing manual-limit terms. However, the zero-margin contact retune is applied through the shared RoughPhysicsCfg even though the removed comment documents that non-AnymalD rough-terrain tasks rely on the previous margin; the robot-specific adjustment is limited to AnymalD.
- API: The new symbol is exported through the MDP API and follows the existing naming convention. Its public docstring does not document max_speed in m/s or describe its parameters in the required Google-style Args section. The new API and task behavior also lack the required per-package changelog fragments.
- Implementation: The termination correctly computes per-body linear speed and reduces violations across selected bodies. However, rigid_contact_max is calculated during post-initialization from the default scene.num_envs, so later CLI/Hydra environment-count overrides leave the buffer over- or under-sized.
Significant concerns. Posted 4 actionable findings inline.
Automated review; human maintainers own approval decisions.
| ) | ||
|
|
||
|
|
||
| def body_lin_vel_out_of_manual_limit( |
There was a problem hiding this comment.
🟡 Warning · Implementation — Missing changelog fragments for touched packages
Repository rules require one fragment per touched package under source/<pkg>/changelog.d/. This PR touches isaaclab (new exported body_lin_vel_out_of_manual_limit) and isaaclab_tasks (physics preset retune, new body_speed termination, AnymalD init pose) but adds none, so these user-visible changes will be omitted from the compiled changelog. Add an Added fragment for the termination and a Changed fragment for the locomotion config updates.
| # terrain — without it, non-AnymalD robots fail to learn stable contact | ||
| # on triangle-mesh terrain. See isaaclab_newton 0.5.22 changelog. | ||
| default_shape_cfg=NewtonShapeCfg(margin=0.01), | ||
| default_shape_cfg=NewtonShapeCfg(margin=0.0, ke=160000.0, kd=1100.0), |
There was a problem hiding this comment.
🟡 Warning · Design Architecture — Shared preset drops margin other robots rely on
RoughPhysicsCfg is inherited by every rough-terrain velocity task, not only AnymalD. The comment deleted from these exact lines stated the 1 cm margin was required for non-AnymalD robots to learn stable contact on triangle-mesh terrain, and the AnymalD-specific override was removed in the same change. Setting margin=0.0 (plus new ke/kd) in the shared preset therefore regresses the other inheriting configs. Keep the shared default and scope the retune to the AnymalD config.
| self.sim.render_interval = self.decimation | ||
| self.sim.physics_material = self.scene.terrain.physics_material | ||
| newton = self.sim.physics.newton_mjwarp | ||
| newton.collision_cfg.rigid_contact_max = newton.solver_cfg.nconmax * self.scene.num_envs |
There was a problem hiding this comment.
🟡 Warning · Implementation — Contact buffer sized from default num_envs
rigid_contact_max is computed in __post_init__, which runs at config construction while scene.num_envs is still the class default (4096). CLI/Hydra --num_envs overrides are applied to the instantiated config afterwards and do not re-run __post_init__, so the buffer stays at 300*4096 regardless: heavily over-allocated for small runs and undersized above 4096 envs. Derive this where num_envs is final.
| def body_lin_vel_out_of_manual_limit( | ||
| env: ManagerBasedRLEnv, max_speed: float, asset_cfg: SceneEntityCfg = SceneEntityCfg("robot") | ||
| ) -> torch.Tensor: | ||
| """Terminate when any of the asset's bodies moves faster than the provided limit.""" |
There was a problem hiding this comment.
🔵 Suggestion · Api — Docstring lacks Args and SI unit
Public API docstrings must be Google-style with an Args: section and inline SI units for physical quantities. This newly exported termination documents neither max_speed ([m/s]) nor asset_cfg, so callers cannot tell how the threshold is interpreted. Add an Args: block using name: description form and annotate the speed unit.
ooctipus
changed the title
Record the new MDP termination and Newton locomotion tuning. Apply Ruff formatting to the ANYmal-D configuration so the required PR checks pass.
285ec69 to
0c20987
Compare
2 participants
Description
This PR tunes the Newton MJWarp preset used by locomotion velocity environments to improve rough-terrain contact stability as part of sim-to-sim policy transfer work.
The changes:
njmax=200andnconmax=100tonjmax=1000andnconmax=300.margin=0.0,ke=160000.0, andkd=1100.0).rigid_contact_maxwithnconmax * num_envsso contact allocation follows the environment count.body_lin_vel_out_of_manual_limitand terminate locomotion episodes when any robot body exceeds 20 m/s.No new dependencies are required.
Performance impact
Increasing
num_substepsfrom 1 to 2 costs approximately 8% performance. This tradeoff is intentional to improve simulation stability.Type of change
Validation
uv run python tools/changelog/cli.py check developuv run isaaclab -fScreenshots
Not applicable.
Checklist
CONTRIBUTORS.md